feat: Add Batch Exports Filters UI #22857
Conversation
📸 UI snapshots have been updated3 snapshot changes in total. 0 added, 3 modified, 0 deleted:
Triggered by this commit. |
|
Size Change: +48 B (0%) Total Size: 1.06 MB ℹ️ View Unchanged
|
📸 UI snapshots have been updated2 snapshot changes in total. 0 added, 2 modified, 0 deleted:
Triggered by this commit. |
📸 UI snapshots have been updated3 snapshot changes in total. 0 added, 3 modified, 0 deleted:
Triggered by this commit. |
…hog into feat/batch-export-filters-2
📸 UI snapshots have been updated1 snapshot changes in total. 0 added, 1 modified, 0 deleted:
Triggered by this commit. |
📸 UI snapshots have been updated1 snapshot changes in total. 0 added, 1 modified, 0 deleted:
Triggered by this commit. |
| return [int(action["id"]) for action in filters.get("actions", [])] | ||
|
|
||
|
|
||
| def hog_function_filters_to_expr(filters: dict, team: Team, actions: dict[int, Action]) -> ast.Expr: |
There was a problem hiding this comment.
It's kind of hard to understand the resulting expression from reading the function. Are there any examples we could look at? Potentially in a test case to ensure this thing works?
There was a problem hiding this comment.
I am mostly interested in the rough shape of the sql an action filter transpiles to, as I have concerns about performance.
There was a problem hiding this comment.
My knowledge here breaks down but maybe @mariusandra can be of help?
It populates to the same expression that a HogQL expression would be, but I'm not sure what the correct way of then using it in a clickhouse query is tbh
There was a problem hiding this comment.
It may be as well that the whole code there doesn't apply for batch exports so we can definitely un-abstract it if thats the case
There was a problem hiding this comment.
Yeah, I did have some follow-up questions regarding how actions would interact with batch exports:
- What if some of the events that compose the action fall in a previous batch? Should that action be included in the current batch? I guess these questions fall under "When does an action happen?".
- Should we extend the schema with an
action_id? - General concerns about performance giving that filter fields may not be in the sort key, and sampling is not a valid strategy for batch exports.
I hoped seeing the SQL would help me clarify these. In the meantime though, it may be easier to limit to event filters only for batch exports, as we already work with those, and they are part of the sort key.
There was a problem hiding this comment.
As far as I understand, actions are just events and properties anyways (other than cohorts but those will not cause a compile error so also don't apply.
If you think of an "Action" as a "Saved Filter" then it is easier to reason on in that I would say it should simply filter the data being exported
|
|
||
| updates = BatchExport.objects.bulk_update(affected_batch_exports, ["filters"]) | ||
|
|
||
| # TODO Publish update to batch export processor |
There was a problem hiding this comment.
The hard part will be updating the processor to work with filters. Publishing will not be possible until that point.
There was a problem hiding this comment.
Yeah, hence the "TODO" 😉
There was a problem hiding this comment.
Yeah, I would be taking this TODO as this looks overall pretty nice, just wanted to clarify the changes are more in the posthog/temporal/batch_exports files than here.
# Conflicts: # frontend/__snapshots__/scenes-app-insights--trends-line-edit--dark.png # frontend/__snapshots__/scenes-app-insights--trends-line-edit--light.png # frontend/src/lib/constants.tsx # frontend/src/scenes/pipeline/hogfunctions/PipelineHogFunctionConfiguration.tsx # frontend/src/scenes/pipeline/hogfunctions/pipelineHogFunctionConfigurationLogic.tsx # latest_migrations.manifest # posthog/models/hog_functions/hog_function.py
📸 UI snapshots have been updated2 snapshot changes in total. 0 added, 2 modified, 0 deleted:
Triggered by this commit. |
📸 UI snapshots have been updated1 snapshot changes in total. 0 added, 1 modified, 0 deleted:
Triggered by this commit. |
📸 UI snapshots have been updated1 snapshot changes in total. 0 added, 1 modified, 0 deleted:
Triggered by this commit. |
📸 UI snapshots have been updated2 snapshot changes in total. 0 added, 2 modified, 0 deleted:
Triggered by this commit. |
📸 UI snapshots have been updated2 snapshot changes in total. 0 added, 2 modified, 0 deleted:
Triggered by this commit. |
📸 UI snapshots have been updated3 snapshot changes in total. 0 added, 3 modified, 0 deleted:
Triggered by this commit. |
|
This PR hasn't seen activity in a week! Should it be merged, closed, or further worked on? If you want to keep it open, post a comment or remove the |
|
This PR hasn't seen activity in a week! Should it be merged, closed, or further worked on? If you want to keep it open, post a comment or remove the |
|
This PR was closed due to lack of activity. Feel free to reopen if it's still relevant. |
Problem
WIP - Adds the same UI as the Hog functions have
Changes
TODO
👉 Stay up-to-date with PostHog coding conventions for a smoother review.
Does this work well for both Cloud and self-hosted?
How did you test this code?